Skip to content

PROXY Protocol support (rd 2) #4505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Conversation

adrum
Copy link

@adrum adrum commented Apr 25, 2025

This is essentially #1882 with the merge conflicts resolved.

I first tried submitting a PR to the source branch, but it's not yet been merged. SBado#1

I thought I might have better luck here 😄

@adrum
Copy link
Author

adrum commented Apr 25, 2025

cc @Iaotle

@adrum
Copy link
Author

adrum commented Apr 26, 2025

Seems like this was a timing issue with the CI during the apt update

@code-a-cola
Copy link

Having the same issue on my pull request.

@beatles1
Copy link

What's the difference between this and #4105 ? This functionality would be really useful for me and just wondering which would be better to help testing for?

@adrum
Copy link
Author

adrum commented Jun 13, 2025

@beatles1 the only difference between this one and the one you linked is this MR layers in a commit on top that resolves the merge conflicts with upstream. Merging this MR should also mark the other one merged.

@beatles1
Copy link

Awesome, hopefully the CI can be re-run and then I'll test the Docker image

@Iaotle
Copy link

Iaotle commented Jun 13, 2025

@adrum I feel kinda bad for rushing you to re-make the PR, you put in effort fixing merge conflicts and submitting a PR just to end up in limbo </3

I admire the effort though!

@AlperShal
Copy link

@jc21 Really sorry for tagging you but looks like this is failing because of a CI issue. And the bot didn't post the details about why CI failed after the build bump. Can you please investigate this?

@NikeLaosClericus
Copy link

I've just started testing this in my homelab (I create pr #4660 just to get a working image).

I can report that I get the same errors that @Coolicky was getting in pr #4105 when enabling PP for Streams specifically when setting the Proxy IP.
When enabling it for Streams without a proxy IP it works fine.
I think this is because the proxy ip lines that are added to _listen.conf:

set_real_ip_from {{ load_balancer_ip }};
real_ip_header proxy_protocol;

Aren't being handled for Stream confs at all (or incorrectly?)

@NikeLaosClericus
Copy link

My next comment is on design choice.

I dislike that fact that enabling PP disables (replaces) the standard web hosting ports altogether.
Yes, PP only works with proxy servers that are using PROXY, but they can live side-by-side without issue in a single server conf on the downstream server. This provides for using both PROXY and non-PROXY connections at the same time.

Mine, manually modified, for example:

server {
...
  listen 80;
  listen 88 proxy_protocol;
  listen 443 ssl;
  listen 444 ssl proxy_protocol;
  server_name example.com;
...
  set_real_ip_from xxx.xxx.xxx.xxx;
  real_ip_header proxy_protocol;
...

And as far as I can tell, the inclusion of the real_ip lines does nothing to harm port 80/443 standard implementations, while still working correctly for 88/444.

This is necessary for my implementation as my upstream proxy server is an nginx Stream configuration using SNI for domain based routing of https/443 connections (without termination).
But nginx does not allow the use of proxy_protocol on; in the http configuration (nor is it required in my case, as I don't need SNI and standard proxying works fine for http connections).

So I need my server configurations in NPM to simultaneously accept 444 PROXY and 80 non-PROXY connections.
Also accepting 443 is great for internally routed connections that don't need to go through my stream proxy, and 88 is good if I ever start using HAProxy (which I understand uses PROXY for both).

@NikeLaosClericus
Copy link

All-in-all, this is absolutely a necessary inclusion for NPM that I'm grateful it's being worked on and is close, but can the implementation be tweaked so that it doesn't 'turn of' standard hosting when enabled, or provide an additional switch to add both if desired?

Unrelated, but it occurred to me that I could have circumvented this by adding an additional proxy host to NPM for the same domains, if NPM allowed per port/protocol configurations.
That way, I could have made the https host PROXY enabled, and the http host non-PROXY.
But seeing as these can live side-by-side in a single server without issue, I think that's probably the best way to do it.

@NikeLaosClericus
Copy link

How I modified /app/templates/_listen.conf, enabling both simultaneously:

{% if enable_proxy_protocol == 1 or enable_proxy_protocol == true%}
  listen 88 proxy_protocol;
  listen 80;
{% if ipv6 -%}
  listen [::]:88 proxy_protocol;
  listen [::]:80;
{% endif %}
{% else -%}
  listen 80;
{% if ipv6 -%}
  listen [::]:80;
{% endif %}
{% endif %}
{% if certificate -%}
{% if enable_proxy_protocol == 1 or enable_proxy_protocol == true%}
  listen 444 ssl proxy_protocol;
  listen 443 ssl;
{% if ipv6 -%}
  listen [::]:444 ssl proxy_protocol;
  listen [::]:443 ssl;
{% endif %}
{% else -%}
  listen 443 ssl;
{% if ipv6 -%}
  listen [::]:443 ssl;
{% endif %}
{% endif %}
{% else %}
  #listen [::]:443;
{% endif %}

@adrum
Copy link
Author

adrum commented Jul 25, 2025

I'll get this PR updated with all this feedback today! Thanks for all the feedback, everyone.

@adrum
Copy link
Author

adrum commented Jul 25, 2025

@NikeLaosClericus I pulled in your changes from #4660 if you want to close it.

@NikeLaosClericus
Copy link

Closed my pr.
I want to be clear that I made no changes in that pr, it was Iaotle's fix on top of your pr.
I was just too impatient to wait.

@nginxproxymanagerci
Copy link

Docker Image for build 4 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-4505

Note

Ensure you backup your NPM instance before testing this image! Especially if there are database changes.
This is a different docker image namespace than the official image.

Warning

Changes and additions to DNS Providers require verification by at least 2 members of the community!

@adrum adrum requested a review from jc21 July 25, 2025 20:35
@NikeLaosClericus
Copy link

I have a better idea about the stream conf issues (#4105 (comment)):

I do not believe @SBado (et al) intended to add this feature to stream configs at all with their original PR.
None of the code changes in the PR seem to (intentionally) cover streams.
(I list some code files below that I think would have included changes if so...)

An example is that lines added to frontend/js/models/proxy-host.js:

enable_proxy_protocol:   false,
load_balancer_ip:        '',

have not been added to frontend/js/models/stream.js in this PR.

The same is true for the rest of the code changes being almost entirely specific to proxy_host files.
I'm not exactly sure what would need to be changed for streams to work,
but here are some other files that may need to be updated:
(because there are equivalent changes in the proxy host files in this PR)

frontend/js/app/nginx/stream/form.(e)js
frontend/js/i18n/messages.json (under the streams section)
backend/templates/stream.conf
backend/schema/paths/nginx/streams/post.json
backend/schema/paths/nginx/streams/get.json
backend/schema/paths/nginx/streams/streamID/get.json
backend/schema/paths/nginx/streams/streamID/put.json
backend/schema/components/stream-object.json
backend/models/stream.js
backend/migrations/???_stream_ssl.js (similar to backend/migrations/20220209144645_proxy_protocol.js)

And accepting that stream support wasn't intended...
Is it possible that however NPM is generating the web page form for proxy host configuration, it's also handling the form for stream configurations and showing options that aren't really configured for streams? (thus the errors)

Given the possible scope of work increase this would represent, maybe it's better just to figure out why the options are being errantly shown on stream confs, and remove them, rather than expand to fit both.

I'm not saying I don't want stream support for PP, but this pr could be quickly accepted as enabling Proxy Protocol support on Proxy Hosts alone, and another PR could handle adding in support for streams later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants